DarkMode (b) DarkModeButtonRenderer #13408
Conversation
KlausLoeffelmann
added
area-DarkMode
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
2 times, most recently
from
57f4d2f to
3b45170
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13408 +/- ##
===================================================
+ Coverage 75.42791% 76.47110% +1.04319%
===================================================
Files 3230 3240 +10
Lines 639213 640319 +1106
Branches 47303 47410 +107
===================================================
+ Hits 482145 489659 +7514
+ Misses 148044 147077 -967
+ Partials 9024 3583 -5441
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
6 times, most recently
from
918e60d to
b83e93b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new dark-mode–aware button rendering system alongside minor tweaks to the toolstrip renderer.
- Adds four sub-renderers (
Standard,Flat,Popup,System) underButtonDarkModeRendererfor WinForms buttons in dark mode. - Updates
ButtonBaseAdapter/ButtonBaseto select and use the new dark-mode adapter when dark mode is enabled. - Toggles a flag in
ToolStripSystemRendererto use the alternative dark-mode path and consolidates pen creation in its dark-mode border renderer.
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/.../ToolStripSystemRenderer.cs | Flip isSystemDefaultAlternative flag to true |
| src/.../ToolStripSystemDarkModeRenderer.cs | Hoist borderPen creation out of branches |
| src/.../Controls/Buttons/DarkModeRenderer/* | New dark-mode renderer classes & factory |
| src/.../Controls/Buttons/DarkModeRenderer/ButtonDarkModeRenderer.cs | Core dispatcher for dark-mode button rendering |
| src/.../ButtonInternal/ButtonDarkModeAdapter.cs | New adapter to route RenderButton calls |
| src/.../ButtonInternal/ButtonBaseAdapter.cs & ButtonBase.cs | Hook up dark-mode adapter via OwnerDraw and CreateDarkModeAdapter |
| Winforms.sln | (Unrelated) added AI-Prompts and GDI solution items |
Comments suppressed due to low confidence (1)
Winforms.sln:212
- Solution file now includes AI-Prompts and GDI entries unrelated to WinForms code. Remove these to avoid polluting the main solution.
Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "AI-Prompts"...
...ndows.Forms/System/Windows/Forms/Controls/Buttons/DarkModeRenderer/ButtonDarkModeRenderer.cs
Outdated
Show resolved
Hide resolved
...ndows.Forms/System/Windows/Forms/Controls/Buttons/DarkModeRenderer/ButtonDarkModeRenderer.cs
Outdated
Show resolved
Hide resolved
| private const int DefaultButtonBorderThickness = 0; | ||
| private const int NonDefaultButtonBorderThickness = 0; |
There was a problem hiding this comment.
[nitpick] Both border-thickness constants are zero, making the switch on isDefault a no-op. Either consolidate into one constant or clarify if different values are intended.
| private const int DefaultButtonBorderThickness = 0; | |
| private const int NonDefaultButtonBorderThickness = 0; | |
| private const int ButtonBorderThickness = 0; |
...s.Forms/System/Windows/Forms/Controls/Buttons/DarkModeRenderer/FlatButtonDarkModeRenderer.cs
Outdated
Show resolved
Hide resolved
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
from
b83e93b to
c505a15
Compare
KlausLoeffelmann
force-pushed
the
klaus/preview5/A
branch
from
b999aad to
56a437d
Compare
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
6 times, most recently
from
61aefaa to
23ff881
Compare
KlausLoeffelmann
changed the base branch from
klaus/preview5/A
to
klaus/preview6/A
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
from
b1bfd78 to
f0e353c
Compare
KlausLoeffelmann
force-pushed
the
klaus/preview6/A
branch
from
35093c5 to
499a4d8
Compare
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
5 times, most recently
from
c0170e5 to
1528a35
Compare
KlausLoeffelmann
force-pushed
the
klaus/preview6/A
branch
from
499a4d8 to
5619f0b
Compare
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
4 times, most recently
from
b993552 to
4a291d5
Compare
KlausLoeffelmann
force-pushed
the
klaus/preview6/A
branch
from
5619f0b to
19fc0c4
Compare
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
3 times, most recently
from
bb6f045 to
6e80645
Compare
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
from
2b83abf to
8a6ad4a
Compare
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
from
8a6ad4a to
1cc912f
Compare
KlausLoeffelmann
force-pushed
the
DarkMode_b_DarkModeButtonRenderer
branch
from
e9a373a to
08204aa
Compare
| /// <term><see cref="FlatStyle.Standard"/></term> | ||
| /// <description> | ||
| /// The default style. The button is not wrapping the system button. It is rendered using the StandardButton adapter. | ||
| /// VisualStyleRenderer from the OS is used for certain parts, which may have issues in high-resolution scenarios. |
There was a problem hiding this comment.
Cref for VisualStyleRenderer?
| /// <term><see cref="FlatStyle.System"/></term> | ||
| /// <description> | ||
| /// The button wraps the system button and is not owner-drawn. | ||
| /// No <c>OnPaint</c>, <c>OnPaintBackground</c>, or adapter is involved. |
There was a problem hiding this comment.
Would be better as a cref for the events.
| /// <description> | ||
| /// The default style. The button is not wrapping the system button. It is rendered using the StandardButton adapter. | ||
| /// VisualStyleRenderer from the OS is used for certain parts, which may have issues in high-resolution scenarios. | ||
| /// Dark mode works to some extent, but improvements are needed. |
There was a problem hiding this comment.
What improvements are needed? As public documentation it would be better to say something like.
| /// Dark mode works to some extent, but improvements are needed. | |
| /// Partially supports dark mode. |
And then make sure we have tracking issues around what we might be able to address.
| /// <description> | ||
| /// The button is fully owner-drawn. No rendering is delegated to the OS, not even VisualStyleRenderer. | ||
| /// This style works well in dark mode and is fully controlled by the application. | ||
| /// 3D effects are expected but may not be rendered; consider revisiting for meaningful styling. |
There was a problem hiding this comment.
What does this mean for the user? Does it really work well if 3d effects don't render? I don't really understand this, but I would guess you might be saying something like:
"Generally, works well in dark mode, with the exception of ..."
There was a problem hiding this comment.
Note, that this is describing a status quo, which has been changing over the time up to a point where the docs often no longer reflect the presents. The docs are still stating things like certain Borderstyle effects (not only for buttons), which are no longer true in general. For example, many controls have a BorderStyle 3D, which no longer resemble any 3D effect. But then we have control like the RichTextBox, which still does that in the same way as under Windows 95.
That was the original motivation behind the Visual Styles, which became then strongly connected to the dark mode changes, and the reason, I would argue to revisit the VisualStyles approach, should it come to it (resources, mandate).
| /// <description> | ||
| /// The button wraps the system button and is not owner-drawn. | ||
| /// No <c>OnPaint</c>, <c>OnPaintBackground</c>, or adapter is involved. | ||
| /// In dark mode, this style is used as a fallback for Standard-style buttons. |
There was a problem hiding this comment.
What are standard style buttons? Is there a cref that could be used here? Or another link that users can follow to understand this from docs?
| /// <summary> | ||
| /// Creates a dark mode compatible brush. Important: | ||
| /// Always do: `using var brush = GetDarkModeBrush(color)`, | ||
| /// since you're dealing with a cached brush => scope, really! |
There was a problem hiding this comment.
Comment is duplicated. Please do not use ! in comments.
There was a problem hiding this comment.
Ah, damn. My bad. Noted.
|
|
||
| if (Application.RenderWithVisualStyles & _useComboBoxTheme) | ||
| #pragma warning disable WFO5001 // Type is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed. | ||
| if (!Application.IsDarkModeEnabled |
There was a problem hiding this comment.
A comment why we're skipping for dark mode would be useful.
| { | ||
| UpdateStyles(); | ||
|
|
||
| // UpdateStyles should also update the UserDraw flag, but it doesn't. |
There was a problem hiding this comment.
"Should" seems to imply that there is a follow up here. Is there something we should be tracking?
There was a problem hiding this comment.
It's maybe phrased a bit awkwardly. We need to update the docs rather. They imply that it should. It's now added to the list, and I ping @adegeo for it and talk to him!
| graphics.DrawPath(focusPen, focusPath); | ||
| } | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
nit: Shouldn't need to add comments to overrides unless you need to change the docs. The less copies of documentation we have around, the less likely we are to have some copies lacking updates.
There was a problem hiding this comment.
Added that rule to the Copilot-Instructions. It should not happen again (if you use that to generate the XML), and I will be working to get that into account for pure refactotoring purposes.
| /// <summary> | ||
| /// Cache of colors for different button states in dark mode. | ||
| /// </summary> | ||
| internal class DarkModeButtonColors |
There was a problem hiding this comment.
Where is this overridden? If it isn't it should be sealed and virtuals removed. In either case, the constructor should not be public.
There was a problem hiding this comment.
It's internal, and I kept the extensibility for us internally in mind. At that time, I did not know, if sealing a class would be final, but if I understood correctly, it is not. Added to the list!
2 participants
Add correlating DarkMode Button renderers to the existing Button renderers.
This way, we are becoming completely independent of the XP-based VisualStyle rendering (and most of all incomplete theming for certain situations, like the Button rendering for 96DPI/100% scaling scenarios).
Fixes #11949.
Note:
For Preview 5, we are very defensively activating the new Renderers, and do a more intensive testing for Preview 6. This means:
Nothing changes, if we're in LightMode. We're not picking up any new Win11 compatible styles for now.
In contrast to the documentation, we're rendering EVERY
FlatModeourselves (and have so all along) - also the defaultFlatMode. That means, ifVisualStylesare enabled, we're still controlling the paint-pipeline (handlingOnPaint), we're just delegating certain parts down to theVisualStyleRenderer(which does not work reliably for dark mode and does not work in 96 DPI altogether.) Our initial assumption that the native Win32 System button doesn't render dark mode correctly is wrong: It does, and it does so in every HighDpi mode (scenario tested by CTI), even with animation support. We are just not using it (and never have), when the user is not explicitly opting in by setting theFlatStyletoSystem. This is what the docs need to be updated for.For that reason, for DarkMode and
FlatStyle.Standard, we're picking up the internalSystemRenderer, except for cases when we're rendering with images, as theSystemRendereris NOT able to render images (and never was). In case of using Images, we're using the new DarkMode FlatRenderer, because that is style-compatible with the SystemRenderer.For the other styles, we're using the new DarkMode renderers respectively (Again, LightMode is not affected), we're not introducing any new Win11 styles for Preview 5.